Repository navigation
HDDS-16314. Filter out blocks still in use before enqueuing overwritten key versions in S3 MPU complete - #11135
Conversation
…en key versions in S3 MPU complete S3MultipartUploadCompleteRequest enqueued the overwritten old key versions to deletedTable without filterOutBlocksStillInUse, unlike OMKeyCommitRequest (HDDS-9146). If the overwritten key shares any block with the newly completed MPU key (e.g. older version groups retained from a versioning- enabled overwrite history), KeyDeletingService would physically delete blocks still referenced by the live key table entry, causing data loss. Apply the same filter in the MPU complete overwrite path; the shared base class covers both the OBS/LEGACY and FSO layouts.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a data-loss risk in the Ozone Manager S3 multipart upload (MPU) complete path when overwriting an existing key: it now filters out blocks that are still referenced by the newly committed key before enqueueing old key versions into deletedTable, aligning MPU-complete behavior with the existing key-commit overwrite protection.
Changes:
- Apply
filterOutBlocksStillInUse(omKeyInfo, oldKeyVersionsToDelete)inS3MultipartUploadCompleteRequest#validateAndUpdateCachebefore adding old versions to the delete list. - Update overwrite expectations in the existing MPU-complete success test (no delete-table entry when the overwritten key has no blocks after filtering).
- Add a new unit test that asserts shared blocks are not enqueued for deletion (and wire it to run for both LEGACY/OBS and FSO via helper overrides).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/multipart/S3MultipartUploadCompleteRequest.java | Adds the in-use block filtering step before enqueueing overwritten key versions for deletion in MPU complete. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartUploadCompleteRequest.java | Adds regression coverage for shared-block overwrite and updates overwrite delete-table expectations. |
| hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/multipart/TestS3MultipartUploadCompleteRequestWithFSO.java | Overrides test helpers so the new shared-block test runs correctly under FSO bucket layout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
@peterxcli thanks for the patch! overall LG left a nits , none blocking
| // container ID and local ID with omKeyInfo blocks'. | ||
| // Otherwise, it causes data loss once those shared blocks are added | ||
| // to deletedTable and processed by KeyDeletingService for deletion. | ||
| filterOutBlocksStillInUse(omKeyInfo, oldKeyVersionsToDelete); |
There was a problem hiding this comment.
nit: could add a line on why the Pair return is ignored here, since OMKeyCommitRequest uses it for quota. MPU already counts the bytes at commit-part.
ivandika3
left a comment
ivandika3
left a comment
There was a problem hiding this comment.
@peterxcli Thanks for the fix. LGTM +1.
What changes were proposed in this pull request?
When an S3 multipart upload completes, add all old key versions to deletedTable, even those that were already deleted, and delete them by KeyDeletingService. If the key has any
ContainerBlockID, add them to deletedTable and delete them by KeyDeletingService. If the key has noContainerBlockID, do not add them to deletedTable.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16314
How was this patch tested?
testOverwrittenKeySharedBlocksNotEnqueuedForDeletioninTestS3MultipartUploadCompleteRequest, with FSO helper overrides so it runs in bothTestS3MultipartUploadCompleteRequestandTestS3MultipartUploadCompleteRequestWithFSO. It completes an MPU over a key table entry that shares one block with the committed part and holds one block of its own, then asserts the deletedTable contains only the old-only block while the completed key still references the shared one. The test fails without the production change.TestS3*unit tests in ozone-manager pass (175 tests), andcheckstyle:checkpasses on the module.Generated-by: Claude Code (claude-fable-5)